feat(tracing): per-step obs wrappers inside business Temporal activities - #491
Merged
Conversation
stainless-app
Bot
force-pushed
the
next
branch
from
August 10, 2026 15:24
462195d to
7ff83aa
Compare
NiteshDhanpal
force-pushed
the
feat/obs-perstep-async-wrapper
branch
from
August 10, 2026 22:00
7b94c34 to
732d739
Compare
…ies (1:1) Previously _begin_obs skipped the obs wrapper for ANY Temporal activity (Option A) and only stamped the ambient RunActivity span, so all business spans in a turn collapsed onto ONE obs span (52:1). But inside a *business* activity, start_span and end_span run in the SAME process, so a wrapper is safe there. Option A is only required for the SDK's own dispatched START_SPAN/END_SPAN activities (the in_temporal_workflow path), where start and end are separate activities on possibly different workers. Discriminate on activity type: _in_tracing_dispatch_activity() is true only for the "start-span"/"end-span" activities. For everything else (sync, or a business activity) open a real per-step wrapper — it nests under the interceptor's ambient RunActivity span and closes in-process, giving each business span its own obs span (1:1), matching the sync path. The bounded _OBS_HANDLES registry backstops any mis-discrimination.
NiteshDhanpal
force-pushed
the
feat/obs-perstep-async-wrapper
branch
from
August 10, 2026 22:18
732d739 to
5decbec
Compare
…tivities
In dd_only (the default), the wrapper branch of _begin_obs read ddtrace for
both open_obs_span and the obs_correlation fallback. Inside a Temporal worker
there is no ddtrace request context, so open_obs_span returned None and the
fallback obs_correlation() returned {} -- every business span in an async turn
persisted with no obs_trace_id/obs_span_id. The ambient span in an activity is
the temporalio OTel TracingInterceptor span regardless of SGP_OBS_MODE (same
reasoning already applied to the dispatch/tag branch).
Fix: thread prefer_otel through the wrapper branch, keyed on "in any Temporal
activity" (restored _in_temporal_activity). open_obs_span gains a prefer_otel
param that opens an OTel wrapper first (falling back to ddtrace). This also
restores real per-step OTel wrapper spans in dd_only business activities, not
just the ids.
Adds a dd_only-inside-a-business-activity regression test (asserts OTel ids win
over ddtrace, not empty); the earlier verification ran in lgtm so it missed this.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The prior tests stubbed _in_tracing_dispatch_activity wholesale, so the actual
`activity.info().activity_type in ("start-span", "end-span")` comparison -- the
one line preventing a cross-worker handle leak inside START_SPAN -- had no
coverage. Add tests that fake activity.info():
- start-span / end-span -> True (asserted against TracingActivityName.value, so
this fails if the enum ever drifts from the strings hardcoded in trace.py),
- a business activity type (process_mortgage_turn) -> False,
- not in an activity -> False (in_activity guard short-circuits before info()),
plus a small _in_temporal_activity() truth-table test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Compare activity.info().activity_type against TracingActivityName.START_SPAN / END_SPAN instead of the string literals "start-span" / "end-span", so the check can't silently drift from the enum that actually names the activities (@activity.defn(name=TracingActivityName.START_SPAN)). Uses a lazy import inside the function to avoid the activities -> TracingService -> AsyncTracer -> trace import cycle (the reason literals were used originally); at call time, inside an activity, the module graph is fully loaded so the import is safe. activity_type round-trips as the enum's str value, which a str-Enum member compares equal to. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…emporal_activity
Deleting _in_temporal_activity earlier dropped the TODO it carried. This PR does
item (a) of it (the named per-step wrapper); the other two are still open, so
re-home them on _begin_obs where the per-step wrapper decision now lives:
(1) TurnTrace RETRY/ASYNC roll-up -- retried turns currently surface as N
per-attempt span sets, not one PRIMARY + N RETRY view.
(2) multi-replica bounded-_OBS_HANDLES + obs_trace_id-resolves-to-turn check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… drift prefer_otel read like a per-call override of SGP_OBS_MODE, which undercut the mode as the source of truth. Rename to expect_otel: it is not a bypass but a context-derived expectation -- "the expected backend here is OTel" -- true on the Temporal path, where the interceptor span is OTel regardless of mode. Keep the mode authoritative and make config-vs-reality drift observable instead of silently absorbing it: warn_on_backend_drift() logs once (deduped per direction) when the expected backend has no active span but the other one does (e.g. dd_only configured but the live span is OTel). Fail-open -- it only warns; the caller still reads and falls back, so no correlation is lost. Wired into _begin_obs on both the dispatch and business-activity paths. Tests: the rename, plus two drift tests (warns once on mismatch; silent when the expected backend is the live one). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… guard The enum import sat above the in_activity() check, so it ran on every start_span -- including the pure-sync ACP path that never touches Temporal. That pulled the whole temporal activities module graph into workflow-less processes, made the docstring's "inside an activity" safety justification untrue for the path actually taken, and meant a broken import would silently return False (disabling dispatch discrimination) even on the sync path. Move it below the guard: sync never runs it, and inside an activity the graph is loaded so the lazy import stays safe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jwl-scale
approved these changes
Aug 11, 2026
harvhan
approved these changes
Aug 11, 2026
…ift; rewrap docstring - warn_on_backend_drift probes the expected backend first and returns early when it is live, so the healthy common path skips the second probe -- and avoids re-attempting the import of a backend that isn't installed (failed imports aren't cached in sys.modules, so the finder cost otherwise recurs every span). Behavior is unchanged. - Rewrap the tag_ambient_obs_span docstring paragraph a prior edit left with one overlong line. Addresses review nits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Two ways to give a business step an obs presence on the Temporal path:
agentex.business_trace_id/agentex.business_span_id) onto the span the OTelTracingInterceptoralready made active (the ambientRunActivityspan). Correlation still works, but the step gets no span of its own.Before this change,
_begin_obsused Option A for any Temporal activity, so every business span in an async turn collapsed onto one obs span, whereas the sync path gives each step its own (1:1). This implements the_in_temporal_activityfollow-up called out in #484.The fix
Option A is only actually required for the SDK's own dispatched
start-span/end-spanactivities (thein_temporal_workflow()path), where start and end run as separate activities that Temporal can route to different workers — so a wrapper opened instart-spancould never be closed byend-span(it would leak and itsobs_span_idwould dangle).Inside a business activity (an agent turn's own
adk.tracing.span, e.g.process_mortgage_turn), start and end run in the same process, so a wrapper is safe there: it nests under the interceptor's ambientRunActivityspan and closes in-process._begin_obsnow discriminates on activity type via_in_tracing_dispatch_activity()(true only forstart-span/end-span):The now-dead
_in_temporal_activity()is removed (its TODO is exactly what this implements). The bounded_OBS_HANDLESregistry backstops any mis-discrimination.Why it matters
With per-step wrappers on the async path you can tell which hop belongs to which step (hops nest under their step, not one giant activity span), each step's persisted
obs_span_idpoints at its own step-named span instead of a shared generic one, and async finally produces the same trace shape as sync.Span volume on retries
A retried business activity now emits a full per-step wrapper set per attempt, each nested under that attempt's
RunActivity(previously they collapsed onto the ambient activity span). This is the intended 1:1 shape, but until the TurnTrace roll-up lands a retried turn surfaces as N per-attempt span sets rather than onePRIMARY+ NRETRYview. That roll-up and a multi-replica bounded-_OBS_HANDLEScheck are tracked asTODO(obs-followup)intrace.py.Verified on infra-staging (rocket-mock-async-agent), one async turn
agentex.businesswrapper spansobs_span_idEach step (
mortgage.advisor.turn,classify_intent,retrieve_docs.kb_query,authz.check.*,tool.*,telemetry.shard.*,synthesis.draft_reply) now gets its own obs span nested underRunActivity, matching the sync path.Tests
Updated
test_temporal_obs_backend.pyfor the new discriminator, plus two tests asserting: inside a dispatch activity → tag ambient (no wrapper); otherwise → open a wrapper.🤖 Generated with Claude Code
Greptile Summary
The PR gives business steps inside Temporal activities dedicated observability wrapper spans while retaining ambient-span tagging for the SDK’s separately dispatched tracing activities.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Begin business span] --> B{Tracing dispatch activity?} B -->|Yes| C[Tag ambient Temporal OTel span] C --> D[Persist ambient correlation IDs] B -->|No| E{Inside any Temporal activity?} E -->|Yes| F[Open per-step OTel wrapper] E -->|No| G[Open wrapper selected by configured mode] F --> H[Persist wrapper correlation IDs] G --> H H --> I[Register handle] I --> J[End business span] J --> K[Close and remove wrapper handle]Reviews (8): Last reviewed commit: "refactor(tracing): probe expected backen..." | Re-trigger Greptile